Set Windows DACL on discovery directory under LOCALAPPDATA - #5951
Set Windows DACL on discovery directory under LOCALAPPDATA#5951stantheman0128 wants to merge 3 commits into
Conversation
os.Chmod is advisory on NTFS, so the discovery directory under %LOCALAPPDATA% could keep inherited ACEs (Everyone, other interactive users). A local attacker who can write server.json can redirect the npipe URL and steal the discovery nonce on the next health check. On Windows, replace the directory DACL with a protected ACL granting GenericAll only to the process user and SYSTEM (create and existing-dir paths). Non-Windows keeps os.Chmod. Windows tests assert no other interactive users remain; POSIX mode assertions skip on Windows. Fixes stacklok#5217 Signed-off-by: stantheman0128 <stanshih888@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
Assignment ack is on #5217. PR is ready for review when you have bandwidth. DCO Signed-off-by is on the commit; Windows DACL tests included. Happy to adjust scope if you want a different approach. Stan Shih (@stantheman0128) |
samuv
left a comment
samuv
left a comment
There was a problem hiding this comment.
Thanks for tackling this. The explicit protected DACL and adversarial Windows tests are a solid direction. I found three security gaps that seem important to resolve before merge: the restriction runs only after startup has already trusted the discovery file, the protected leaf remains replaceable through the intermediate directory, and hostile ownership can survive the DACL replacement. I left focused inline comments with suggested fixes and regression coverage.
One threat-model wording note: the current health request does not transmit the discovery nonce. The concrete risks are endpoint spoofing, persistent startup denial, and interception or forgery of later client API traffic, so the PR and commit description should be adjusted accordingly.
Checklist
- Tests: Good Windows create and replace coverage; additional production-order, ancestor-replacement, and hostile-owner tests are needed. The PR currently has no checks, and the repository test workflow is Ubuntu-only.
- Docs: No user-facing documentation appears necessary, but the nonce-leak wording should be corrected.
- Registry impact: None.
- Security: Requesting changes for the three inline findings.
- Backwards compatibility: The non-Windows behavior and error text appear preserved; unsupported Windows filesystems remain an explicit fail-closed compatibility tradeoff.
| return fmt.Errorf("failed to set discovery directory permissions: %w", err) | ||
| // On Windows this sets an explicit protected DACL (POSIX modes are | ||
| // advisory on NTFS); elsewhere it is os.Chmod(dirPermissions). | ||
| if err := restrictDiscoveryDirPermissions(dir); err != nil { |
There was a problem hiding this comment.
Could we move this restriction earlier, before the startup path acquires server.json.lock and calls discovery.Discover? In pkg/api/server.go, writeDiscoveryFile currently does MkdirAll → WithFileLock → Discover → WriteServerInfo, so an existing loose directory is trusted before this function runs. An attacker can write a server.json with an attacker-controlled pipe and nonce, return that nonce from /health, and make Discover report StateRunning; the caller then returns before WriteServerInfo, so the DACL is never repaired. I think the permission setup needs to happen before the lock and Discover, with a regression test covering that production ordering.
| // | ||
| // Inheritance (OICI) is intentional: server.json and any future children | ||
| // pick up the same restriction instead of inheriting a looser parent ACL. | ||
| func restrictDiscoveryDirPermissions(dir string) error { |
There was a problem hiding this comment.
Could we also protect or validate the intermediate toolhive directory? The adversarial test grants Everyone inheritable Modify on the parent and then creates parent\toolhive\server, but this function restricts only the server leaf. That leaves toolhive with inherited Modify; Modify includes DELETE on that object and directory-creation rights in its parent, so another user can rename toolhive aside and recreate toolhive\server, bypassing the protected leaf. Please secure the relevant path chain, or enforce and document a restrictive-ancestor invariant, and extend the Windows test to cover ancestor replacement.
| return fmt.Errorf("failed to build discovery directory DACL: %w", err) | ||
| } | ||
|
|
||
| if err := windows.SetNamedSecurityInfo( |
There was a problem hiding this comment.
Could we validate the directory owner before considering the lockdown complete? This call replaces only the DACL and passes a nil owner. If another user pre-created the directory and granted the process enough rights for this operation to succeed, that user remains the owner; Windows owners retain the ability to change the DACL and can make it permissive again. Please either fail closed when the owner is not the process user or securely establish the expected owner, and add coverage for a pre-existing directory with hostile ownership.
Review follow-up. The DACL work only ran inside writeServerInfoTo, which is the last step of startup: pkg/api creates the directory, takes server.json.lock inside it, and runs Discover before it ever writes. A StateRunning result returns before the write, so a directory left loose by an earlier run stayed loose for the whole startup, and the server.json in it was trusted regardless. Move the lockdown into discovery.EnsureSecureDir and call it from writeDiscoveryFile ahead of the lock and Discover. Restrict the intermediate toolhive directory as well as the server leaf: inherited Modify on the intermediate carries DELETE, so another interactive user could rename it aside, recreate the chain with an ACL of their own, and leave the protected leaf unread. Validate ownership before treating a directory as locked down. Owners keep WRITE_DAC implicitly, so replacing the DACL of a directory somebody else pre-created is cosmetic, since that owner can make it permissive again. Fail closed unless the owner is the process user, SYSTEM, or Administrators; a standard user cannot create a directory owned by any of those, and an elevated thv serve legitimately leaves Administrators as owner. Apply the same check to server.json on the read path: changing the parent DACL re-propagates inherited ACEs but leaves explicit ACEs and ownership alone, and that file's URL and nonce decide where clients send API traffic. Correct the threat model this branch described. The health request does not transmit the nonce, so a spoofed endpoint learns nothing. The risks are endpoint spoofing, persistent denial of startup, and interception or forgery of later client API traffic. Non-Windows behavior is unchanged: os.Chmod over the same chain, and the owner check is a documented no-op because POSIX modes are enforced rather than advisory. Tests: production ordering (Discover reports StateRunning, startup aborts, both directories are already protected), ancestor replacement (intermediate and leaf both protected), and hostile ownership (fail closed with the loose ACE left intact, plus the read-path rejection). Fixes stacklok#5217 Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: stantheman0128 <stanshih888@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
size/L
The helper listed the SDDL aliases of the groups a local attacker could come from and asserted their absence. Enumerate the ACEs and require every trustee to be SYSTEM or the process user instead: that also covers trustees the list did not name, and it drops a bare alias literal that codespell reads as a misspelling. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: stantheman0128 <stanshih888@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
size/L
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5951 +/- ##
==========================================
+ Coverage 71.80% 72.53% +0.73%
==========================================
Files 708 735 +27
Lines 72767 75939 +3172
==========================================
+ Hits 52250 55084 +2834
- Misses 16778 16936 +158
- Partials 3739 3919 +180 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks for the review. All three findings held, and the wording note was right. Ordering. Intermediate directory. Both Ownership. Checked before the DACL write, so a directory somebody else owns is never half-secured, and again afterwards, so an owner that races the write cannot be reported as a completed lockdown. Fail closed rather than take ownership: taking ownership would repair the container while leaving the planted Your findings together surfaced one more thing: with the directory fixed, an already-planted Wording. Agreed and corrected in the PR body. Tests. Added production-order ( Pushed Stan Shih (@stantheman0128) |
2 participants
Summary
writeServerInfoTocreated the discovery directory underxdg.StateHome(%LOCALAPPDATA%on Windows) withos.MkdirAll+os.Chmod(0700). POSIX modes are advisory on NTFS, so inherited ACEs from the parent (Everyone, other interactive users) survive, and any local account holding Modify can rewriteserver.json. With thenpipe://discovery URL from #5201 / #5214, that file is what decides where the next client connects.What changed:
server.jsoninherits the same restriction. Non-Windows keepsos.Chmod.discovery.EnsureSecureDir, called fromwriteDiscoveryFilebeforeserver.json.lockis taken and beforeDiscoverruns, instead of only inside the write.%LOCALAPPDATA%\toolhiveand%LOCALAPPDATA%\toolhive\server.readServerInfoFromrejects aserver.jsonowned by another account, alongside the existing symlink rejection.Fixes #5217
Threat model (corrected)
The first revision of this description claimed a spoofed endpoint could capture the discovery nonce. That is wrong, and @samuv is right to flag it:
CheckHealthsends a plainGET /healthwith no nonce in the request and compares the nonce that comes back in the response header, so nothing is transmitted to the endpoint being probed. An attacker who can rewriteserver.jsonpicks the nonce themselves.The actual risks of an attacker-writable discovery file are:
server.jsonand dial the attacker's pipe or address./healthwith the matching nonce makesDiscoverreportStateRunning, so everythv serveaborts with "another ToolHive server is already running".Review round 2: the three findings
1. The restriction ran after startup had already trusted the directory (
discovery.go:83).writeDiscoveryFiledidMkdirAllthenWithFileLockthenDiscoverthenWriteServerInfo, so the DACL landed last. Two consequences:server.json.lockwas created in a directory other accounts could still write to, and aStateRunningresult returns beforeWriteServerInfois ever reached, so the directory stayed loose for the whole startup while its planted contents were believed.discovery.EnsureSecureDir()now runs first, andWriteServerInfoalso calls it so the public write path cannot regress.TestWriteDiscoveryFile_RestrictsDirBeforeTrustingExistingFiledrives the realwriteDiscoveryFilewith a planted, healthy-looking file and asserts the chain is already protected on the failing return. Without the reordering that test fails withD:AI(A;OICIID;0x1301bf;;;WD)...on both directories (output below).2. The intermediate
toolhivedirectory is protected too (permissions_windows.go:23).Correct: restricting only the leaf left the intermediate with inherited Modify, and Modify carries DELETE on that object, so another interactive user could rename
toolhiveaside and recreatetoolhive\serverwith an ACL of their own.EnsureSecureDirwalks the chain outermost first and restricts each directory ToolHive creates.Above that chain there is a documented invariant rather than more ACL rewriting, because
%LOCALAPPDATA%belongs to the OS and to other applications: ToolHive relies on its default protected ACL (user, SYSTEM, Administrators). If another account does hold delete-child there, it can still swap the chain aside, and the ownership check in finding 3 is what turns that into a startup failure instead of silent trust. That reasoning is in a comment at the top ofpermissions_windows.go.3. Ownership is validated, fail closed (
permissions_windows.go:58).Also correct, and it was the sharpest of the three: owners keep WRITE_DAC implicitly, so a DACL written onto a directory somebody else pre-created is cosmetic.
restrictDiscoveryDirnow checks the owner before writing the DACL (so a hostile directory is never half-secured) and again afterwards (so a pre-creating owner that raced the write cannot be reported as a completed lockdown).Trusted owners are the process user, SYSTEM, and Administrators. Administrators is included deliberately: a directory created by an elevated
thv serveis owned by Administrators rather than by the user, and a local administrator is outside this threat model since they can take ownership of anything. It does not weaken the check against the attacker in scope, because a standard user cannot make SYSTEM or Administrators the owner of a directory they create. Taking ownership instead of failing was considered and rejected: it would silently repair the container while leaving the attacker'sserver.jsoninside it to be trusted.One thing the three findings surfaced together: an already-planted
server.jsonis trusted even with the directory fixed, so the read path needed the same treatment. Changing the parent DACL does re-propagate inherited ACEs to existing children (visible in the Evidence below), but explicit ACEs and ownership are untouched, soreadServerInfoFromnow refuses a discovery file owned by another account. On startup the effect is self-healing rather than fatal:Discoversurfaces the error, startup proceeds on its existing "discovery check failed" path, andAtomicWriteFilereplaces the planted file with one that inherits the protected DACL.Type of change
Test plan
go test ./pkg/server/discovery/ ./pkg/api/on Windows 11, go1.26.5;taskis not installed on this machine)task test-e2e)golangci-lint runat v2.6.2 with the repo.golangci.yml, forGOOS=linuxandGOOS=windows;go vetfor linux, darwin, windows)%LOCALAPPDATA%product path, below)Evidence
Production ordering, before the fix. Reverting only the
pkg/apireordering and running the new test shows exactly the gap that was reported: startup aborts on the planted file and both directories are still inheriting (D:AI) with Everyone (WD) on them.Unit tests, after the fix (Windows 11, go1.26.5):
Product path: real
%LOCALAPPDATA%, realdiscovery.EnsureSecureDir. The chain was pre-created loose (Everyone inheritable Modify on the base, plainMkdirAllfor the children) with a plantedserver.json, the way an earlier run plus a local attacker would leave it.Before:
After
discovery.EnsureSecureDir():Both directories end up
D:P(protected) with the user and SYSTEM only, on both the intermediate and the leaf. The pre-existingserver.jsonshows the inherited-ACE re-propagation mentioned above: Everyone is gone from it without the file being rewritten. That is also why the read path checks ownership rather than trusting the ACL alone.What was not tested
thv servebinary end to end. There is no container runtime on this machine (docker infofails, andpkg/apitests that build the full server already fail onno available runtime foundatorigin/main), so startup aborts before it reaches the discovery step. The newpkg/apitest calls the realwriteDiscoveryFileon the real path instead, and the product-path Evidence above uses the real exporteddiscovery.EnsureSecureDiragainst%LOCALAPPDATA%.SeRestorePrivilege, so the hostile-owner tests inject the trusted-owner set against a directory the test user really owns, which exercises the same failure path.task test/task lint-fix(Task is not installed here).golangci-lint runwas run directly with the repo config for bothGOOS=linuxandGOOS=windows: the only findings are three pre-existinggcireports on files this PR does not touch (pkg/api/docs.go,openapi.go,scalar.go), caused by CRLF in this local Windows checkout.pkg/server/discoveryon this machine are pre-existing and identical atorigin/main: the two symlink tests (os.SymlinkneedsSeCreateSymbolicLinkPrivilege) and three unix-socket path tests. This branch turns threeorigin/mainfailures into skips (the POSIX-mode assertions).API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.Changes
pkg/api/server.gowriteDiscoveryFilecallsdiscovery.EnsureSecureDir()before the lock andDiscoverpkg/api/discovery_permissions_windows_test.gopkg/server/discovery/discovery.goEnsureSecureDir, directory chain helper, owner check on the read pathpkg/server/discovery/permissions_windows.gopkg/server/discovery/permissions_other.goos.Chmodplus a no-op owner checkpkg/server/discovery/permissions_windows_test.gopkg/server/discovery/discovery_test.goDoes this introduce a user-facing change?
On Windows,
%LOCALAPPDATA%\...\toolhiveand itsserversubdirectory are locked down to the user that startedthvplus SYSTEM, and this now happens at the start ofthv serverather than at the end. Two new fail-closed startup errors are possible where startup previously proceeded silently: a discovery directory owned by another account, and aserver.jsonowned by another account. Both name the path and say what to do. No CLI flag or config change.Special notes for reviewers
thv servehas ever been run elevated.npipe://URL.%LOCALAPPDATA%before and after ACLs, and confirmed the pre-fix failure by reverting the reordering.Made with Cursor